MapQuest improvement #840

TerjeBr · 2018-04-03T15:52:51Z

This is an improvment to the MapQuest provider, that we talked about in #839

I have tried to stay away from the code in the Common directory, but I could not avoid some small minor changes there to make it all fit together. Hope this will be ok.

Nyholm · 2018-04-03T16:10:35Z

Thank you for this PR.
I did a quick review and I see some issues when using a chain provider.
Do you really need a custom implementation of a query?

TerjeBr · 2018-04-03T16:16:46Z

The custom implementation of a query is only to make it easier to use. It will work fine with a reqular query as well, as I wrote in the Readme.md

Nyholm · 2018-04-03T16:49:21Z

I would be way happier without the custom implementation. That would also avoid the changes in Common

TerjeBr · 2018-04-03T17:15:44Z

I am sorry that you do not like the custom implementation of GeocodeQuery. Is there something in particular that bothers you about it? Something I can improve?

Or are you just opposed to having two GeocodeQuery implementations in general?

The additional one should work fine as a replacement for all the cases where the original one may be used.

Without the new GeocodeQuery implementation, you first have to create a GeocodeQuery with a dummy address string (it cannot be empty), and then clone it in order to have the address data on it. I found this to be a bit cumbersome, so that is why I added the new implementation.

TerjeBr · 2018-04-03T22:02:25Z

It is a bit unfortunate that the interface Geocoder\Provider\Provider (that all the providers must implement) depends on the concrete classes Geocoder\Query\GeocodeQuery and Geocoder\Query\ReverseQuery instead of interfaces. When in addition those concrete classes are final it leaves no room for changing or improving the Query classes that are used in a graceful way.

I guess changing it to use interfaces is the "right" thing to do, but that will be a huge BC break. Removing the final keyword on the concrete classes is less invasive. If you choose to do that, you also might as well make the properties protected instead of private.

The only other alternative I can see is to declare that the Query classes are set in stone, and we can have no suggested changes or development on the Query classes. I think that would be a bit unfortunate thing to do.

TerjeBr · 2018-04-13T16:54:56Z

Hi @Nyholm what/when will be the next step?

Nyholm

Thank you for the reminder. I gave this another quick review.

About the massive changes in MapQuest: You are the expert here. I do not know much about that provider and the API.

About the changes in Common: I would be really happy if they are kept to a minimum.

Nyholm · 2018-04-13T17:03:18Z

src/Common/Query/GeocodeQuery.php

    /**
     * @param string $text
     */
-    private function __construct(string $text)


Why do you need to override a private constructor? Why not override the create function?

It seemed to me to be logical that when the class is not final any more, child classes should be free to do a different __construct. But I can do as you say, and override the create function instead, no problem.

I would like to keep the changes to the GeocodeQuery to a strict minimum.

Nyholm · 2018-04-13T17:08:15Z

src/Provider/MapQuest/MapQuest.php

-        return $this->executeQuery($url);
+    private function extractAddressFromQuery(CommonGeocodeQuery $query)
+    {
+        if ($query instanceof GetAddressInterface) {


Sorry for my lack of understanding. Why do we need this check?

Dont we get the address either way? The way you implemented getAddress is:

public function getAddress() { return $this->getData(static::DATA_KEY_ADDRESS); }

This was my attempt at future proofing the code a bit.
This way for any GeocodeQuery object that implements the GetAddressInterface will just call the getAddess method, and we do not care where it is stored. If I could have changed the common GeocodeQuery class I would have left it at that.

The line

return $query->getData(GeocodeQuery::DATA_KEY_ADDRESS);

is just a fallback for if you are using a regular GeocodeQuery that does not implement the GetAddressInterface.

I think it is better to depend on interfaces instead of depending on a known constant in a specific class.

I implemented getAddress the way I did to have max interoparability with the common GeocodeQuery class, so that a programmer did not have to use the mapquest version of the GeocodeQuery class.

Nyholm · 2018-04-13T17:09:09Z

src/Common/Query/GeocodeQuery.php

    /**
     * @var array
     */
-    private $data = [];


Why do you need to modify the scope of $data? You could just introduce a new property on Geocoder\Provider\MapQuest\GeocodeQuery, right?

My goal was that the new GeocodeQuery class could be a drop in and exchangeable with the old one. If I introduce a new property, I would also have to reimplement the withData method right?

It seemed easier for me to do it this way. If you agree to remove the final keyword, why cannot all properties be protected?

Nyholm · 2018-04-13T17:09:42Z

src/Common/Query/GeocodeQuery.php

     *
     * @var string
     */
-    private $text;


Is this needed? If you overwrite withText then there is no need for changing this to protected, right?

Yes, I can overwrite all the methods to avoid changing the properties in the base class to from private to protected. But I thought the proper way was to change the visibilty of the properties, not to duplicate the code in the methods?

If you change the visibility like this we risk that someone writes code to make this mutable. I would like you not to change this.

Nyholm · 2018-04-13T17:11:25Z

src/Common/Query/GeocodeQuery.php

 /**
 * @author Tobias Nyholm <tobias.nyholm@gmail.com>
 */
-final class GeocodeQuery implements Query


This hurts a little but Im fine with it =)

Thank you for that. I am glad that you will at least allow this.

Nyholm · 2018-04-13T17:11:44Z

src/Http/Provider/AbstractHttpProvider.php

+        return $this->parseHttpResponse($response, $url);
+    }
+
+    protected function parseHttpResponse(ResponseInterface $response, string $url): string


Thank you for this function.

You are welcome.

Nyholm · 2018-04-13T17:12:17Z

src/Provider/MapQuest/GeocodeQuery.php

+        return $this->text;
+    }
+
+    protected function formatAddress(Location $address): string


This function seams to violate the single responsibility principle.

How is that?

It is a protected method required by the above getText() method.
Should not a query containing an address have a method to convert that address to a one line text query, so that the query can be used also in other providers?

This is only meant as a default value for the text of the query, and will not be called if the user calls GoecodeQuery::withText him/her self to set the text portion of the query.

(Btw. "seams" = sömmar, "seems"= verkar)

Nyholm · 2018-04-13T17:15:24Z

src/Provider/MapQuest/GeocodeQuery.php

+        return new self($address);
+    }
+
+    public function getAddress()


This function is only use internally. It could be removed.

This is the implementation of the GetAddressInterface. It makes is possible to get the address out of the Query without knowing anything about how it is stored in the Query.

What do you mean it is only used internally? Other (user land) code code also have use of the ease of calling getAddress on the Query, if they want to get the addres from the query.

Also, since this GecodeQuery has a createFromAddress method, is it not natural that it then also has getAddress method?

This reminds me that may be I should have an withAddress method as well, in case the user wants to add the address or "change" it later. (Yes, I know it is an immutable object that does not change :-)

What do you think, should I add the method withAddress?

Nyholm · 2018-04-13T17:17:10Z

src/Provider/MapQuest/GeocodeQuery.php

+
+    public static function createFromAddress(Location $address): self
+    {
+        return new self($address);


Just do:

$foo = new self(''); return $foo->withData(static::DATA_KEY_ADDRESS], $address);

It is a bit more ineffecient, as it will create two objects instead of one, and then leave the first one to be garbage collected.
But yes, that is a valid way to do it if the methods in the base class cannot be changed to be protected instead of private.

Another thing is that $foo = new self('') will not work, it would trigger the exception InvalidArgument('Geocode query cannot be empty').

And if I have to put something in the $text property, what should it be? Since you want me to remove the formatAddress method I would be stuck with a GeocodeQuery that would return something weird if it is used in another provider that calls getText.

It is really not that inefficient. We are talking optimization on less than duoble quotes vs single quotes.

Ok, forget my inefficency arguments then. Please respond to the other points I raised.

TerjeBr · 2018-04-14T01:30:02Z

Thank you for your review. I will wait for your response before I push a new set of changes.

TerjeBr · 2018-04-26T08:58:47Z

@Nyholm any updates on this? I am still waiting for your comments.

TerjeBr · 2018-05-14T09:41:45Z

@Nyholm Have you been on vacation or something?

Please continue the conversation so we can land this PR.

TerjeBr · 2018-05-14T10:11:32Z

@Nyholm if we are going to change the Common\Query\GeocodeQuery class from final to non-final, do you not agree that it makes sense to also change the constructor and all the properties from private to protected?

Do you agree with the above statement? Or do you have any reason I am not aware of for wanting to keep it all as private?

Nyholm

Yes, I've been to vacation. =)

I really like the changes you do in the provider. The review process of this PR is is harder because you want to push many changes to the GeocodeQuery class. I find it hard to see scenarios where you really have to update that class. Most of your arguments are "it is easier"...

Try you restrict yourself to not update the common objects. You may send another PR with these changes later.

Nyholm · 2018-05-14T09:57:20Z

src/Common/Query/GeocodeQuery.php

     *
     * @var string
     */
-    private $text;


If you change the visibility like this we risk that someone writes code to make this mutable. I would like you not to change this.

Nyholm · 2018-05-14T10:22:47Z

src/Common/Query/GeocodeQuery.php

    /**
     * @param string $text
     */
-    private function __construct(string $text)


I would like to keep the changes to the GeocodeQuery to a strict minimum.

TerjeBr · 2018-05-14T10:43:42Z

Ok, If you will please accept #849 that I have made with the minimal changes to the common files, I will then after that make the necessary changes to this PR to reflect that.

Hopefully we can then move this PR forward.

TerjeBr · 2018-05-14T13:00:10Z

@Nyholm for me to make the tests pass you need to tag a new version of https://github.com/geocoder-php/provider-integration-tests

TerjeBr · 2018-05-15T12:53:22Z

@Nyholm plase make a new version tag on the geocoder-php/provider-integration-tests repository so that I can update the composer.lock files so this will do the tests properly.

You will also need to merge #849 and add a tag to that before this PR can show green on all tests.

TerjeBr · 2018-05-16T08:15:21Z

@Nyholm, have you gone on vacation again?

TerjeBr · 2018-05-17T18:23:39Z

@Nyholm Are you celebrating Norways national day today?
Is that why you are not doing anything with this PR today?

TerjeBr · 2018-05-21T12:23:54Z

@Nyholm ping

TerjeBr · 2018-05-22T14:47:34Z

I have complied with everything you have requested, as far as I know.

If you want to merge just this pull request, that is fine, then you can ignore #849
Or if you want do do it in stages, you can merge #849 first, and then this one.

Please tell me if there is anything else you need for this to be merged.

Nyholm · 2018-05-22T15:30:45Z

Please tell me if there is anything else you need for this to be merged.

I'll try to be extra clear with what I mean.

This is the process I want to follow:

You update this PR so you do not modify any classes in common
I review it and after I'm happy I'll merge it
(optional) You make a PR with changes in common that you can prove would significantly improve maintainability and user experience for at least a handful of providers.

TerjeBr · 2018-05-22T16:02:36Z

Does that mean you are backtracking on what we already have discussed?

On 13th of April you agreed that I could at least change the Common\Query\GeocodeQuery by removing the final keyword. All the other changes I can code around, but I cannot code around that.

Less important are the changes to Http\Provider\AbstractHttpProvider (you even thanked me for adding a function there) and a small bugfix in Common\Model\AddressBuilder.
That is all the changes that you will find in #849

So, have you changed your stance so to not allow any changes at all in the common classes now?

TerjeBr · 2018-05-24T10:27:23Z

And again we have this great wall of silence.

Is it too much to expect that you are able to respond to me within a day or so?
Does it have to be weeks between when we exchange comments on this PR? I find that very frustrating.

Nyholm · 2018-05-24T10:42:16Z

And again we have this great wall of silence.

Im doing open source work on my free time because I enjoy it. Doing open source is however not the only thing Im doing before and after normal work.
Im not sure if I intrepid your messages correctly, but I sense a hostile tone. (Correct me if Im wrong.) That makes it no fun to spend time helping you get your PR merged.

So, have you changed your stance so to not allow any changes at all in the common classes now?

I've not changed my mind. But I feel that you do not really listen when I say "restrict yourself from making changes in common." I know it is possible to add the features you want to add without any changes at all in common (as explained). I want you to see that as well. Since I want to help you to get this merged, I now ask you to not make any changes at all in common since that will ease reviewing and the decisions to merge is not too critical for the rest of the project.

You may think Im unfair or unreasonable. I do not want to discuss this anymore, but Im happy to review an updated PR.
If other maintainers think do not agree with me you are more welcome to say so.

TerjeBr · 2018-05-24T10:49:06Z

I know it is possible to add the features you want to add without any changes at all in common (as explained).

This is not true.

It is impossible for me to continue without at least removing the final keyword from the Common\Query\GeocodeQuery class. I thought we already discussed that, and that you agreed to that change.

All the other ones I can code around, but not that one.

TerjeBr · 2018-05-24T10:51:04Z

Shall I go ahead and make the PR so that the removing of the final keyword is the only change among the common files?

TerjeBr · 2018-05-24T11:07:24Z

You are aware that I updated this PR on 14th of May right? And that that update made it only contain 3 small changes to the common files that I thought we were in agreement on?

If you were not aware of that, that may be the cause that we are talking past each other?

TerjeBr · 2018-05-25T16:13:42Z

@Nyholm please answer me so I can go ahead with this PR

TerjeBr · 2018-06-10T12:08:12Z

Since you do not give any answers, I will just have to assume. I assume then that I reached the correct conclusion when I said "make the PR so that the removing of the final keyword is the only change among the common files". I will go ahead and do just that.

TerjeBr · 2018-06-10T13:14:02Z

Ok now everything should be as you want it.

Please tag a new version for https://github.com/geocoder-php/provider-integration-tests
if you want all the tests to pass before you merge.

unkind · 2018-06-18T02:49:35Z

Without the new GeocodeQuery implementation, you first have to create a GeocodeQuery with a dummy address string (it cannot be empty), and then clone it in order to have the address data on it. I found this to be a bit cumbersome, so that is why I added the new implementation.

Do I understand right that you want to remove final just because you're missing named constructor like GeocodeQuery::location(...)?

Maybe it's better to improve GeocodeQuery model instead of inheriting it?

unkind · 2018-06-18T02:52:23Z

@Nyholm argh, removing final is a very bad sign in most cases. You can call me next time :)

unkind · 2018-06-18T03:29:39Z

After reading original topic, I think you can just make (static) function to create GeocodeQuery for MapQuest: MapQuest::createQuery(...).

By the way, why does Geocoder have Query interface? I thought you decided you throw it off: 40bdb8d

Nyholm · 2018-06-26T16:29:49Z

I've added a PR to your PR: TerjeBr#1

TerjeBr · 2018-07-08T20:48:36Z

Ok, I am back from my vacation. Let's see what has happened here.

TerjeBr · 2018-07-08T20:58:04Z

@unkind wrote:

Do I understand right that you want to remove final just because you're missing named constructor like GeocodeQuery::location(...)?

Maybe it's better to improve GeocodeQuery model instead of inheriting it?

You are right that it would be better to improve GeocodeQuery instead of inheriting it. That is what I asked about first in #839, if there could be a way to incorporate the address in the query. But that seems to be a no go from @Nyholm.

So, this quest of mine to inherit from GeocodeQuery is my attempt to circumvent that so mapquest users could have at least some level of user friendliness in the use of this liberary. But now that hope seems to be thwarted too, since he has obviously changed his mind and wont even allow the final keyword to be removed from the class (that he previously agreed to).

With these changes I find it way easier to merge your PR

TerjeBr · 2018-07-08T21:22:18Z

Ok, I hope this can finally be merged now then.

It fails the style guide check, but that was in the changes that @Nyholm introduced, so I guess that is ok.

TerjeBr · 2018-07-20T10:10:57Z

Ping

Nyholm

Great. Thank you

willdurand requested a review from Nyholm April 3, 2018 15:52

TerjeBr changed the title ~~Map quest improvement~~ MapQuest improvement Apr 3, 2018

TerjeBr mentioned this pull request Apr 3, 2018

Add body content to cache key when it is a POST request geocoder-php/provider-integration-tests#12

Merged

TerjeBr mentioned this pull request Apr 5, 2018

Locale-aware FreeGeoIp provider #829

Merged

Nyholm reviewed Apr 13, 2018

View reviewed changes

Nyholm requested changes May 14, 2018

View reviewed changes

TerjeBr pushed a commit to TerjeBr/Geocoder that referenced this pull request May 14, 2018

Changes to common files for PR geocoder-php#840

9b736e2

TerjeBr mentioned this pull request May 14, 2018

Changes to common files for PR #840 #849

Closed

TerjeBr pushed a commit to TerjeBr/Geocoder that referenced this pull request May 14, 2018

Changes to common files for PR geocoder-php#840

065b84f

TerjeBr force-pushed the map-quest-improvement branch from 400434b to 4112648 Compare May 14, 2018 13:18

TerjeBr pushed a commit to TerjeBr/Geocoder that referenced this pull request May 14, 2018

Changes to common files for PR geocoder-php#840

ac144cc

TerjeBr force-pushed the map-quest-improvement branch from 4112648 to 97a0ff6 Compare May 14, 2018 13:20

Changes to common files for PR geocoder-php#840

3022c12

TerjeBr force-pushed the map-quest-improvement branch 2 times, most recently from fcd816b to b9d4e35 Compare June 10, 2018 13:05

MapQuest support 5Box address specification

c05351a

TerjeBr force-pushed the map-quest-improvement branch from d95a437 to c05351a Compare June 10, 2018 13:10

With these changes I find it way easier to merge your PR

e46b874

TerjeBr and others added 2 commits July 8, 2018 22:07

Merge pull request #1 from Nyholm/map-quest-improvement

e384853

With these changes I find it way easier to merge your PR

Let us keep the method extractAddressFromQuery

4bc7cdd

Update Readme.md

f76224f

Nyholm approved these changes Jul 20, 2018

View reviewed changes

Nyholm merged commit 1fdce51 into geocoder-php:master Jul 20, 2018

MapQuest improvement #840

MapQuest improvement #840

Uh oh!

Conversation

TerjeBr commented Apr 3, 2018

Uh oh!

Nyholm commented Apr 3, 2018

Uh oh!

TerjeBr commented Apr 3, 2018

Uh oh!

Nyholm commented Apr 3, 2018

Uh oh!

TerjeBr commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TerjeBr commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TerjeBr commented Apr 13, 2018

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerjeBr Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerjeBr Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerjeBr Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerjeBr commented Apr 14, 2018

TerjeBr commented Apr 3, 2018 •

edited

Loading

TerjeBr commented Apr 3, 2018 •

edited

Loading

TerjeBr Apr 14, 2018 •

edited

Loading

TerjeBr Apr 14, 2018 •

edited

Loading

TerjeBr Apr 14, 2018 •

edited

Loading

TerjeBr commented May 14, 2018 •

edited

Loading

TerjeBr commented May 15, 2018 •

edited

Loading

TerjeBr commented May 22, 2018 •

edited

Loading